Wrangle benchmarks #289

kazimuth · 2023-09-12T19:26:54Z

Description of Changes

Rewrite benchmarks to allow direct comparison between:

spacetime_module: Spacetime via a module
spacetime_raw: Spacetime via the RelationalDB struct
sqlite

And add some targeted serialization benchmarks.

kazimuth · 2023-09-18T16:18:28Z

benchmarks please

kulakowski

Awesome. Inline is mostly minor comments about comments. I think this PR can land whenever, as far as I'm concerned, but I want two things.

One is the framework or schema or strategy or whatever you want to call it, by which you derived the set of benchmarks to write out. If Joshua (say) adds an Update operation that isn't just a delete+insert (say), I'd love for him to be able to read that framework and just slot in exactly the right set of benchmarks. I'd like this to be part of this PR.

Two, if there's a ways to go, I'd like other people to be able to see either in the code or in a ticket how far there is to go until this work is complete. I'm happy if there are more PRs following this one until you get there.

crates/bench/README.md

crates/testing/src/modules.rs

kulakowski · 2023-09-22T17:36:42Z

crates/core/src/host/wasm_common/module_host_actor.rs

+    fn clear_table(&self, table_name: String) -> Result<(), anyhow::Error> {
+        let db = &*self.worker_database_instance.relational_db;
+        db.with_auto_commit(|tx| {
+            let tables = db.get_all_tables(tx)?;


We could plumb through a table_by_name function?

kulakowski · 2023-09-22T17:37:18Z

crates/core/src/host/wasm_common/module_host_actor.rs

+            let tables = db.get_all_tables(tx)?;
+            for table in tables {
+                if table.table_name != table_name {
+                    continue;


We should also return once we find it. This probably can also be expressed as a call to find or whatever on tables.iter, if we don't do a table_by_name approach.

I wasn't sure if there could be multiple tables with the same name here, can there be?

kulakowski · 2023-09-23T07:35:49Z

crates/bench/src/sqlite.rs

-    fn build(prefill: bool, fsync: bool) -> ResultBench<Self>
+pub struct SQLite {
+    db: Connection,
+    _temp_dir: TempDir,


The finickiness of the TempDir drop stuff makes me want a comment here, we've had enough bugs in tests and CI about it that I don't want this refactored away to a local or whatever.

In fact I think refactoring it to a local could cause the tempdir to be dropped, which might break things... then again, the earlier benchmarks version did that and the sqlite benches still worked. So idk.

kulakowski · 2023-09-23T07:36:42Z

crates/bench/src/sqlite.rs


-        if prefill {
-            prefill_data(&mut db, Runs::Small)?;
+    fn create_table<T: BenchTable>(&mut self, table_style: crate::schemas::TableStyle) -> ResultBench<Self::TableId> {


Mario or someone might be a better reviewer of the sqlite stuff here than me.

kulakowski · 2023-09-23T07:38:00Z

crates/bench/src/spacetime_raw.rs

+    }
+
+    type PreparedFilter = PreparedFilter;
+    #[inline(never)]


I picked this #[inline(never)] one arbitrarily out of all of them to comment: you should describe where and why you do this in some top level place if possible!

I added them all in in an attempt to improve compile times, but it didn't have a huge effect, because everything is still heavily monomorphizing due to the generics. I'll strip them out.

kulakowski · 2023-09-23T07:39:23Z

crates/bench/src/spacetime_module.rs

+
+pub struct SpacetimeModule {
+    runtime: Runtime,
+    // it's necessary for this to be dropped BEFORE the runtime.


I personally prefer this (an explicit Drop impl) to e.g. relying on a comment about relying on the drop ordering based on field order. Maybe Mazdak or Noa have a stronger style opinion about it, I'd be curious.

crates/bench/src/schemas.rs

kazimuth · 2023-09-25T16:36:47Z

@kulakowski, I'll address your comments and then I think this is ready for merge.

One is the framework or schema or strategy or whatever you want to call it, by which you derived the set of benchmarks to write out. If Joshua (say) adds an Update operation that isn't just a delete+insert (say), I'd love for him to be able to read that framework and just slot in exactly the right set of benchmarks. I'd like this to be part of this PR.

Good point, I'll document this.

Two, if there's a ways to go, I'd like other people to be able to see either in the code or in a ticket how far there is to go until this work is complete. I'm happy if there are more PRs following this one until you get there.

Yeah, I wasn't really sure where I was going while working on this which meant I had a hard time communicating what I was doing. I have some more targeted benchmark ideas which I'll stick in a followup PR.

I do have one remaining concern, which is benchmark time. Looking at the tests on this PR, it seems that running benchmarks on GitHub actions currently takes about 80 minutes. We could:

disable the in-memory benchmarks on GitHub actions, which would get rid of one axis of this giant hypercube. Alternatively, the sqlite benchmarks, since these won't change. I think both of those provide useful information, though.
Reduce benchmark warm-up and sampling times. This would increase variance.
Just accept that benchmarks are slow.

kazimuth · 2023-09-25T16:44:02Z

Oh, also, should I rename "Person" to "IntStringInt" and "Location" to "IntIntInt" or something similar? Having mysterious schema names in benchmark results might confuse people.

kazimuth · 2023-09-25T18:54:01Z

I have a path to fix benchmark times -- it's because everything is being run twice, including the sqlite benchmarks. I'll mess with our Github Actions benchmark script to fix that.

crates/bench/src/sqlite.rs

mamcx

LGTM

crates/bench/src/sqlite.rs

These calls unconditionally panicked before anyway; now they just panic in a way that Clippy doesn't object to.

kazimuth force-pushed the kazimuth/benchwrangle branch 3 times, most recently from 62e0a3c to f797782 Compare September 15, 2023 18:39

kazimuth mentioned this pull request Sep 15, 2023

Benches refactor #131

Closed

3 tasks

kazimuth force-pushed the kazimuth/benchwrangle branch from fef1058 to 73e591b Compare September 18, 2023 18:17

kazimuth added 2 commits September 20, 2023 13:42

Major benchmarks refactoring

c590daf

Fix compile after cherry-pick

303b332

kazimuth force-pushed the kazimuth/benchwrangle branch from c3e555d to 303b332 Compare September 20, 2023 17:46

kazimuth added 5 commits September 20, 2023 14:55

More cleanup

39dc052

Add some serialization benchmarks

e0847b9

Refactor

09cd8c2

Provoke github actions

ffc0b20

Benchmark action needs clippy

1751388

kulakowski approved these changes Sep 23, 2023

View reviewed changes

kazimuth added 2 commits September 26, 2023 12:26

Simplify bench database interface

ec97cf5

Address review comments

3ea638f

mamcx requested changes Sep 26, 2023

View reviewed changes

crates/bench/src/sqlite.rs Show resolved Hide resolved

crates/bench/src/sqlite.rs Show resolved Hide resolved

kazimuth added 3 commits September 27, 2023 13:14

Better comments, s/table_style/index_strategy

73c5d5d

Fix filesystem issues (names too long)

7253f65

Minor edits

464e75a

kazimuth enabled auto-merge (squash) September 27, 2023 19:32

mamcx approved these changes Sep 27, 2023

View reviewed changes

crates/bench/src/sqlite.rs Show resolved Hide resolved

gefjon added 2 commits September 29, 2023 13:56

Merge branch 'master' into kazimuth/benchwrangle

de50aed

Replace now-removed Table::delete calls with unimplemented!

80ef57f

These calls unconditionally panicked before anyway; now they just panic in a way that Clippy doesn't object to.

kim mentioned this pull request Sep 29, 2023

Fix bootstrapping of ST_MODULE #342

Merged

4 tasks

Merge branch 'master' into kazimuth/benchwrangle

022261d

kazimuth merged commit 010c7e3 into master Sep 29, 2023

joshua-spacetime mentioned this pull request Nov 27, 2023

Build a “tx / s” benchmark #601

Closed

bfops pushed a commit that referenced this pull request Jul 17, 2025

Fix typo (#289)

fade50a

Wrangle benchmarks #289

Wrangle benchmarks #289

Uh oh!

Conversation

kazimuth commented Sep 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of Changes

Uh oh!

kazimuth commented Sep 18, 2023

Uh oh!

kulakowski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kazimuth commented Sep 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kazimuth commented Sep 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kazimuth commented Sep 25, 2023

Uh oh!

Uh oh!

Uh oh!

mamcx left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kazimuth commented Sep 12, 2023 •

edited

Loading

kazimuth commented Sep 25, 2023 •

edited

Loading

kazimuth commented Sep 25, 2023 •

edited

Loading